Fix modified_beam_search hallucination/empty text for NeMo TDT#3657
Fix modified_beam_search hallucination/empty text for NeMo TDT#3657kittysoftpaw0510 wants to merge 2 commits into
Conversation
Fixes three bugs in OfflineTransducerModifiedBeamSearchNeMoDecoder that caused ~20% of transcriptions to produce hallucinated output or empty text when using modified_beam_search with Parakeet-TDT models: 1. Missing blank+skip==0 guard: When the TDT model predicted blank with duration 0, the hypothesis stayed on the same frame with num_symbols reset to 0, causing an infinite loop that produced empty or hallucinated output. Now blank always advances by at least 1 frame, matching the greedy decoder. 2. duration_log_prob incorrectly added to blank candidates: The duration probability was applied uniformly to all candidates. For blank tokens the decoder state is unchanged and frame advancement follows different logic, so including the duration score skewed beam ranking. Now only non-blank candidates include the duration component. 3. max_symbols_per_frame was 10 vs greedy decoder 5: The higher limit allowed more spurious token emissions per frame. Aligned with the greedy TDT decoder. Fixes k2-fsa#3267
dosubot
Bot
added
the
size:M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReduce per-frame non-blank emissions to 5 in TDT mode and refactor beam expansion: blank/unk use token-only scoring with forced frame advance ≥1 and reset per-frame symbol count; non-blank use token+duration scoring, advance by predicted_skip, update decoder state and num_symbols, and apply context-graph scoring during non-blank expansion. ChangesTDT Hypothesis Scoring and Frame Advancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)sherpa-onnx/csrc/offline-transducer-modified-beam-search-nemo-decoder.ccsherpa-onnx/csrc/offline-transducer-modified-beam-search-nemo-decoder.cc:5:10: fatal error: 'sherpa-onnx/csrc/offline-transducer-modified-beam-search-nemo-decoder.h' file not found ... [truncated 1382 characters] ... tem" "/usr/local/include" "-internal-isystem" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the offline transducer modified beam search decoder for NeMo models, reducing the maximum symbols per frame and refining the scoring and frame-advancing logic for TDT models to align with the greedy decoder. A review comment suggests making the reduction of max_symbols_per_frame conditional on is_tdt_ to avoid potential regressions in non-TDT models.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| int32_t vocab_size = model_->VocabSize(); | ||
| int32_t blank_id = vocab_size - 1; // NeMo models have blank at the end | ||
| int32_t max_symbols_per_frame = 10; | ||
| int32_t max_symbols_per_frame = 5; // Match greedy TDT decoder limit |
There was a problem hiding this comment.
Reducing max_symbols_per_frame to 5 unconditionally might cause regressions or search errors (deletion errors) for non-TDT NeMo models, which previously used a limit of 10. It is safer to make this limit conditional on is_tdt_ so that non-TDT models retain their original behavior.
| int32_t max_symbols_per_frame = 5; // Match greedy TDT decoder limit | |
| int32_t max_symbols_per_frame = is_tdt_ ? 5 : 10; // Match greedy TDT decoder limit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sherpa-onnx/csrc/offline-transducer-modified-beam-search-nemo-decoder.cc`:
- Line 97: The tight per-frame symbol cap was applied unconditionally by setting
max_symbols_per_frame = 5; restrict this TDT-only change by gating it on is_tdt_
(or revert to the prior default for non-TDT paths) so non-TDT branches in
OfflineTransducerModifiedBeamSearchNemoDecoder that intentionally allow repeated
same-frame emissions (see the non-TDT handling around the repeated-emission
logic and the zero-cost force-advance/ predicted_skip use) are unaffected;
update the initialization of max_symbols_per_frame to use the smaller value only
when is_tdt_ is true (or assign the previous larger default when !is_tdt_), and
ensure the TDT-specific std::max(1, predicted_skip) guard remains applied only
for the TDT path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7f74c68-b324-4b71-8496-eb641e406404
📒 Files selected for processing (1)
sherpa-onnx/csrc/offline-transducer-modified-beam-search-nemo-decoder.cc
Non-TDT NeMo models intentionally allow up to 10 same-frame emissions. Lowering the cap unconditionally could cause deletion errors for those models. Keep 10 for non-TDT, use 5 only for TDT (matching its greedy decoder). Addresses review feedback from k2-fsa#3657. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thank you for your contribution! Could you provide a test WAV file that reproduces the bug this PR is intended to fix? |
2 participants
Fixes #3267
Problem
modified_beam_searchwith NeMo TDT models (e.g. Parakeet-TDT-0.6b-v2) produces hallucinated output ("Yeah.") or empty text ~20% of the time.Root Cause
Three bugs in
OfflineTransducerModifiedBeamSearchNeMoDecoder, identified by comparing against the working greedy decoder (DecodeOneTDT):1. Missing blank+skip==0 guard (primary cause)
The greedy TDT decoder has a critical safety check:
The beam search decoder was missing this. When blank was predicted with duration 0, the hypothesis stayed on the same frame with
num_symbolsreset to 0, creating a loop that produced empty or hallucinated output.2.
duration_log_probadded to blank candidatesThe duration probability was applied to ALL candidates uniformly. For blank tokens, the decoder state is unchanged and frame advancement uses different logic -- including the duration score skewed beam ranking toward blank-heavy hypotheses that raced through frames without emitting tokens.
3.
max_symbols_per_frame= 10 vs greedy decoder 5The higher limit allowed more spurious token emissions per frame, contributing to hallucinated short phrases like "Yeah."
Changes
duration_log_probstd::max(1, predicted_skip)frames (explicit guard)max_symbols_per_framereduced from 10 to 5 to match greedy TDT decoderTest plan
modified_beam_searchon audio that previously produced "Yeah." or empty textSummary by CodeRabbit